Respect canonical import comments #1017

rtfb · 2017-08-15T19:41:33Z

What does this do / why do we need it?

This adds capability to pkgtree to extract canonical import path comments from the packages. That will later be used as additional constraints when solving the dependencies (will come in a separate PR).

(And a couple unrelated typo fixes and code cleanups along the way)

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

I was wondering, at the very end of ListPackages, where the package gets added to the Packages map, should I also add a copy with a canonical path key like blow? So that solver could see both versions available?

if pkg.CommentPath != "" {
	ptree.Packages[pkg.CommentPath] = PackageOrErr{
		P: pkg,
	}
}

Also, is there a way to develop from my fork? Can't even get tests running:

$ go test $(go list ./internal/gps/pkgtree/...)
# github.com/rtfb/dep/internal/gps/pkgtree
internal/gps/pkgtree/pkgtree_test.go:20:2: use of internal package not allowed
FAIL	github.com/rtfb/dep/internal/gps/pkgtree [setup failed]

It works fine on the main golang/dep repo, but then transplanting pieces of code to another repo before committing is tedious.

Which issue(s) does this PR fix?

This is phase 1 for issue #902.

We already import and use build package here, so there's no harm in reuse. The code is both shorter and more readable.

Look for import comments following the package declaration. The implementation of findImportComment() very closely follows go/build's findImportComment(). Tests and validation against importRoot to come in a later commit.

Added a check and an error type for importRoot to be a prefix of canonical import path, tests for the new code paths. Fixed one unrelated bug along the way: Sprintf("%q", strings.Join(strs, "\", \"")) doesn't work as intended, since the %q verb will escape the inner doublequotes.

googlebot · 2017-08-15T19:47:25Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

rtfb · 2017-08-16T05:38:10Z

I signed it!

…

On Tue, Aug 15, 2017 at 10:47 PM, googlebot ***@***.***> wrote: Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 *Please visit https://cla.developers.google.com/ <https://cla.developers.google.com/> to sign.* Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks. ------------------------------ - If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data <https://cla.developers.google.com/clas> and verify that your email is set on your git commits <https://help.github.com/articles/setting-your-email-in-git/>. - If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot. - In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1017 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAaBZEBDCv9dJbAtma0GMaHgRCuZJznfks5sYfXSgaJpZM4O4BPx> .

-- Vytautas Šaltenis On the Web: http://rtfb.lt/

googlebot · 2017-08-16T05:38:13Z

CLAs look good, thanks!

sdboyer · 2017-08-16T15:13:09Z

woohoo! so glad we're making progress on this.

Also, is there a way to develop from my fork? Can't even get tests running:

yep, the way i do it is to develop directly in $GOPATH/src/github.com/golang/dep, but add a second remote that points to my fork. i use my fork as origin, and this repo as upstream.

should I also add a copy with a canonical path key like blow? So that solver could see both versions available?

nope, we don't want that. the goal of ListPackages() is to provide an accurate picture of what's actually on disk - a virtual representation of the filesystem. we rely on that design principle in a lot of places; adding an entry that undermines that rule would just confuse things.

i'll try to do a review in the next day or two 😄

sdboyer

sorry sorry it took me so long! the mechanics of this look pretty good, but we need to change a bit about how the return semantics work.

sdboyer · 2017-08-17T03:54:55Z

internal/gps/pkgtree/pkgtree.go

+}
+
+// ConflictingImportComments indicates that the package declares more than one
+// different canonical paths.


nit: s/paths/path/

sdboyer · 2017-08-17T03:55:28Z

internal/gps/pkgtree/pkgtree.go

+}
+
+// NonCanonicalImportRoot reports the situation when the dependee imports a
+// package via something else that the package's declared canonical path.


nit: s/else that/other than/

sdboyer · 2017-09-05T01:45:52Z

internal/gps/pkgtree/pkgtree.go

@@ -152,18 +154,23 @@ func ListPackages(fileRoot, importRoot string) (PackageTree, error) {
 			}
 		}

+		if pkg.CommentPath != "" && !strings.HasPrefix(pkg.CommentPath, importRoot) {
+			return &NonCanonicalImportRoot{


this needs softening a bit, as right now, the only cases where we return an actual error from ListPackages() itself (which is what this would result in) are more physical than logical - e.g., we can't actually read the filesystem. the rest of the API is built around that kind of assumption, too.

so, instead of returning the error directly, let's make this an error that we put in the PackageOrErr.Err slot on each package where we encounter the problem.

I presume an equivalent change should be made to prevent ConflictingImportComments from leaking as well?

rtfb · 2017-09-10T11:03:52Z

sorry sorry it took me so long!

No worries, I haven't been able to spend significant time on it either :-). Just enough to address your feedback...

AppVeyor tests are failing at something that doesn't seem to be related. Can this be due to my branch falling too far behind? Should I rebase?

sdboyer · 2017-09-13T02:37:12Z

it wasn't you, it was other problems we were having. appveyor's been whipped back into line, now 😄

sdboyer

back, after more delay! basically wording and comment nits, but the mechanics look good. lots more that we could add with this later.

sdboyer · 2017-09-19T23:57:44Z

internal/gps/pkgtree/pkgtree.go

@@ -210,6 +220,7 @@ func fillPackage(p *build.Package) error {

 	var testImports []string
 	var imports []string
+	var importComments []string


ok so i was originally going to suggest that we just keep a single string, rather than a []string, and upon encountering later values, just compare against that one string and error out immediately if we find an error. i was thinking that it might escape, but i ran it through with the optimization profiler (-gcflags '-m -m'), and turns out, it doesn't.

so, this is fine. just thought you might be interested that i checked that through 😄

sdboyer · 2017-09-20T00:43:29Z

internal/gps/pkgtree/pkgtree.go

+}
+
+func quotedPaths(ps []string) string {
+	var quoted []string


microoptimization nit: quoted := make([]string, 0, len(ps))

sdboyer · 2017-09-20T01:05:56Z

internal/gps/pkgtree/pkgtree.go

+// ConflictingImportComments indicates that the package declares more than one
+// different canonical path.
+type ConflictingImportComments struct {
+	ImportPath     string


Because the provenance of these errors can be a little confusing, let's include comments explaining the intent of the information recorded in each of the struct fields here, analogous to Package.

sdboyer · 2017-09-20T01:06:20Z

internal/gps/pkgtree/pkgtree.go

+// different canonical path.
+type ConflictingImportComments struct {
+	ImportPath     string
+	ImportComments []string


For more clarity, let's name this ConflictingImportComments.

sdboyer · 2017-09-20T01:13:01Z

internal/gps/pkgtree/pkgtree.go

+// NonCanonicalImportRoot reports the situation when the dependee imports a
+// package via something other than the package's declared canonical path.
+type NonCanonicalImportRoot struct {
+	ImportRoot string


As with the above, let's include brief field-by-field comments on these to explain where they come from.

sdboyer · 2017-09-20T01:16:50Z

internal/gps/pkgtree/pkgtree.go

+}
+
+func (e *NonCanonicalImportRoot) Error() string {
+	return fmt.Sprintf("importing via path %q, but package insists on a canonical path %q",


i think this phrasing may end up being a bit confusing, as it would come out:

importing via path "github.com/foo/Bar", but package insists on a canonical path "github.com/foo/bar/bleep/blorp/bopkins"

(case variation here used only to provide a clear example, this probably isn't how it'll be seen most of the time)

i don't have a specific wording suggestion, but we should tweak this a little bit to accommodate the fact that we're contrasting a root path with what could potentially be a subpackage path.

sdboyer · 2017-10-06T04:38:47Z

bump

rtfb · 2017-10-06T06:02:38Z

Yeah, I'm on it. I was travelling when you last commented, and came around to sit down with it couple days back, only to find out that I was locked out of my GH 2FA during phone transition. You know, little joys of life :-)

I'll try to push an update later today.

rtfb · 2017-10-06T18:16:48Z

The build failed only on 1.9.x, with a lint error, while on others it passed. Weird :-/

sdboyer · 2017-10-07T00:32:14Z

no worries - thanks for keeping at this! and yeah, we only run the linters on the 1.9 travis build. it should just be that one issue, then: https://travis-ci.org/golang/dep/jobs/284321611#L547 - the newline byte slice var wasn't used.

i've no idea why the CLA bot is misbehaving. it's been doing this for a week or so now :(

gmlewis · 2017-10-07T05:18:37Z

I apologize for the CLA problems - we are working on resolving the issues. I've rescanned this PR and it is now cleared.

sdboyer · 2017-10-07T11:36:38Z

@gmlewis oh, i appreciate the update - and i didn't even have to go looking for anybody to ping about it! 👍 😄

sdboyer

great! i think this is ready to go 🎉

rtfb added 4 commits August 14, 2017 21:38

Cleanup: fix typos

d6140bd

Cleanup: simplify switch statement, reuse build.IsLocalImport

0e41a07

We already import and use build package here, so there's no harm in reuse. The code is both shorter and more readable.

Implement import comment extraction

19c7f60

Look for import comments following the package declaration. The implementation of findImportComment() very closely follows go/build's findImportComment(). Tests and validation against importRoot to come in a later commit.

rtfb requested a review from sdboyer as a code owner August 15, 2017 19:41

googlebot added the cla: no label Aug 15, 2017

googlebot added cla: yes and removed cla: no labels Aug 16, 2017

sdboyer suggested changes Sep 5, 2017

View reviewed changes

Record NonCanonicalImportRoot error instead of immediate return

90d2a5e

ibrasho assigned ibrasho and unassigned ibrasho Sep 9, 2017

ibrasho added the area: gps label Sep 9, 2017

Fix NonCanonicalImportRoot test

1853dc3

jmank88 closed this Sep 10, 2017

jmank88 reopened this Sep 10, 2017

sdboyer suggested changes Sep 20, 2017

View reviewed changes

Address feedback: naming, wording and comments

47e42c7

rtfb force-pushed the respect-import-comments-issue-902 branch from 5198b48 to 47e42c7 Compare October 6, 2017 17:50

Fix lint

611e64b

sdboyer approved these changes Oct 7, 2017

View reviewed changes

sdboyer merged commit 667d662 into golang:master Oct 7, 2017

darkowlzz changed the title ~~[WIP] Respect canonical import comments~~ Respect canonical import comments Oct 7, 2017

darkowlzz mentioned this pull request Oct 10, 2017

Move bad go code under a vendor/ subdirectory. #1253

Closed

sdboyer mentioned this pull request Oct 14, 2017

Trim hidden packages from (root) PackageTrees #1271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Respect canonical import comments #1017

Respect canonical import comments #1017

rtfb commented Aug 15, 2017 •

edited

Loading

googlebot commented Aug 15, 2017

rtfb commented Aug 16, 2017 via email

googlebot commented Aug 16, 2017

sdboyer commented Aug 16, 2017

sdboyer left a comment

sdboyer Aug 17, 2017

sdboyer Aug 17, 2017

sdboyer Sep 5, 2017

rtfb Sep 10, 2017 •

edited

Loading

rtfb commented Sep 10, 2017

sdboyer commented Sep 13, 2017

sdboyer left a comment

sdboyer Sep 19, 2017

sdboyer Sep 20, 2017

sdboyer Sep 20, 2017

sdboyer Sep 20, 2017

sdboyer Sep 20, 2017

sdboyer Sep 20, 2017

sdboyer commented Oct 6, 2017

rtfb commented Oct 6, 2017

rtfb commented Oct 6, 2017

sdboyer commented Oct 7, 2017

gmlewis commented Oct 7, 2017

sdboyer commented Oct 7, 2017 •

edited

Loading

sdboyer left a comment

Respect canonical import comments #1017

Respect canonical import comments #1017

Conversation

rtfb commented Aug 15, 2017 • edited Loading

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

googlebot commented Aug 15, 2017

rtfb commented Aug 16, 2017 via email

googlebot commented Aug 16, 2017

sdboyer commented Aug 16, 2017

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rtfb Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

rtfb commented Sep 10, 2017

sdboyer commented Sep 13, 2017

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer commented Oct 6, 2017

rtfb commented Oct 6, 2017

rtfb commented Oct 6, 2017

sdboyer commented Oct 7, 2017

gmlewis commented Oct 7, 2017

sdboyer commented Oct 7, 2017 • edited Loading

sdboyer left a comment

Choose a reason for hiding this comment

rtfb commented Aug 15, 2017 •

edited

Loading

rtfb Sep 10, 2017 •

edited

Loading

sdboyer commented Oct 7, 2017 •

edited

Loading